Begin transition to Microsoft.CodeAnalysis.Testing #41717

sharwell · 2020-02-15T01:02:19Z

Add new verification helpers
Convert AddAccessibilityModifiersTests to the new test library

sharwell · 2020-02-15T01:04:25Z

src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs

        }

        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
        public async Task TestRefStructs()
        {
-            await TestInRegularAndScriptAsync(@"


Testing in script code is not currently supported, because diagnostics in script code fail to include the document path in the diagnostic.

this is fine with me.

src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs

src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs

src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2.cs

src/EditorFeatures/TestUtilities/CodeActions/VisualBasicCodeFixVerifier`2+Test.cs

src/EditorFeatures/TestUtilities/CodeActions/VisualBasicCodeFixVerifier`2.cs

…agnosticTest

mavasani · 2020-02-18T17:37:12Z

src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs

 namespace Outer
 {
    namespace Inner1.Inner2
    {
-        internal class {|FixAllInDocument:C|}
+        internal partial class [|C|] : I


Why partial?

➡️ Compile errors were fixed in ec82406 to allow for a constrained review. This change addresses the CS0751 error.

mavasani · 2020-02-18T17:45:46Z

src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs

+                        options = options.WithChangedOption(key, value);
+                    }
+
+                    solution = solution.WithOptions(options);


I believe this approach of options testing will not work for analyzers ported to CodeStyle layer. Solution/workspace options are currently not passed down to the AnalyzerConfigOptionSet which means they will be unavailable in CI. I understand we would like to eventually pass these down, but until that is implemented, these tests will not work in code style layer. This led to me eventually use the approach here: https://github.com/dotnet/roslyn/pull/41363/files#r378505952.

I can confirm this once this PR goes in, and I will try to rebase #41363 on top of this verifier. If required, I will port similar logic here.

I believe this approach of options testing will not work for analyzers ported to CodeStyle layer

I traced the original implementation through the base test classes and mirrored the approach (i.e. this is a direct translation of the original tests). Changes may be required for the CodeStyle layer, but even if necessary would be constrained to the two helpers.

mavasani · 2020-02-18T17:46:54Z

src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs

+
+            protected override AnalyzerOptions GetAnalyzerOptions(Project project)
+            {
+                return new WorkspaceAnalyzerOptions(base.GetAnalyzerOptions(project), project.Solution);


FYI: WorkspaceAnalyzerOptions type is not available in CodeStyle layer.

WorkspaceAnalyzerOptions is required for the current analyzer implementations. Translation to CodeStyle layer equivalents would be left for the porting process.

mavasani · 2020-02-18T17:51:46Z

src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs

-        private IDictionary<OptionKey, object> OmitDefaultModifiers =>
-            OptionsSet(SingleOption(CodeStyleOptions.RequireAccessibilityModifiers, AccessibilityModifiersRequired.OmitIfDefault, NotificationOption.Suggestion));
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
+        public void TestStandardProperties()


Should we use the approach used in current test framework where there is a common base type for all diagnostics tests which have these common tests, so that every new analyzer gets these tests without having to explicitly remember to author them:

roslyn/src/EditorFeatures/TestUtilities/Diagnostics/AbstractDiagnosticProviderBasedUserDiagnosticTest.cs

Lines 56 to 119 in e704ca6

[Fact]

public void TestSupportedDiagnosticsMessageTitle()

{

using (var workspace = new AdhocWorkspace())

{

var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1;

if (diagnosticAnalyzer == null)

{

return;

}

foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics)

{

if (descriptor.CustomTags.Contains(WellKnownDiagnosticTags.NotConfigurable))

{

// The title only displayed for rule configuration

continue;

}

Assert.NotEqual("", descriptor.Title?.ToString() ?? "");

}

}

}

[Fact]

public void TestSupportedDiagnosticsMessageDescription()

{

using (var workspace = new AdhocWorkspace())

{

var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1;

if (diagnosticAnalyzer == null)

{

return;

}

foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics)

{

if (ShouldSkipMessageDescriptionVerification(descriptor))

{

continue;

}

Assert.NotEqual("", descriptor.MessageFormat?.ToString() ?? "");

}

}

}

[Fact(Skip = "https://github.com/dotnet/roslyn/issues/26717")]

public void TestSupportedDiagnosticsMessageHelpLinkUri()

{

using (var workspace = new AdhocWorkspace())

{

var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1;

if (diagnosticAnalyzer == null)

{

return;

}

foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics)

{

Assert.NotEqual("", descriptor.HelpLinkUri ?? "");

}

}

}

These tests are covered by one new test in each analyzer test class:

[Fact] public void TestStandardProperties() => VerifyCS.VerifyStandardProperties();

If we have a problem remembering to add this test, we can add an analyzer for it.

📝 If we want to break out this test into each of the individual properties to match the old code, we could use this instead:

[Fact, CombinatorialData] public void TestStandardProperty(StandardAnalyzerProperty property) => VerifyCS.VerifyStandardProperty(property); public enum StandardAnalyzerProperty { Title, Message, HelpLinkUri, }

mavasani

Overall, this looks very promising step forward, though we might need more tweaks as we support more scenarios.

sharwell added 2 commits February 14, 2020 16:58

Implement analyzer and fix verifiers

4a7d283

Convert AddAccessibilityModifiersTests to the new test library

6495e6f

sharwell requested review from a team as code owners February 15, 2020 01:02

sharwell commented Feb 15, 2020

View reviewed changes